[Fix] Reset correct priority on disinherit timeout #1338
Conversation
This commit corrects vTaskPriorityDisinheritAfterTimeout to reset the previously inherited priority when the task disiheriting timeout was the only task at that priority. Without this change the ready list for the inherited priority will remain set when no task is ready at that priority. This can have consequences later as the ready priority flags are assumed to be accurate.
|
Closing as this will break correct behavior. See explanation on #1337. |
|
Reopening after further discussion on #1337 |
|
jasonpcarroll
left a comment
jasonpcarroll
left a comment
There was a problem hiding this comment.
Approved - this is the correct behavior. The mutex holder has uxPriorityUsedOnEntry as the priority it will disinherit. When it disinherits, it needs to be removed from the ready list of the priority it inheritted if its there (lines 6902->6904). When its removed from this ready list, if that ready list is empty, it needs to be communicated to the port that there are no longer any tasks ready at uxPriorityUsedOnEntry due to the disinheritance. Great find. I imagine that maybe this was missed becasue at one point the code pxTCB->uxPriority as the inheritted priority variable (makes sense), but on line 6883 we set this to PriorityToUse (the priority it should be after the disinheritance occurs).
|
Honestly - we should really rework the variable naming a bit here ... also its a bit confusing as to why we set pxTCB to pxMutexHolder directly... esp since both are used in the function.. makes it hard to understand what is happening. |
|
Perhaps we should also add some UTs that ensure portRESET_READY_PRIORITY is called with the correct priorities for functions that call it ? I.e. just stub it with our own def that just asserts it is called with the correct priorities when the system is in a known state. It worries me that before/after this change no UTs were added and none failed either - how will we catch things like this in the future? |
|
@jasonpcarroll UTs are easy to add but will need to be done in a separate PR since they live in FreeRTOS/FreeRTOS. They do not fail because they do not verify the |
2 participants
Description
This commit corrects vTaskPriorityDisinheritAfterTimeout to reset the previously inherited priority when the task disiheriting timeout was the only task at that priority. Without this change the ready list for the inherited priority will remain set when no task is ready at that priority. This can have consequences later as the ready priority flags are assumed to be accurate.
Thanks @wirelinker for bringing this to the teams attention!
Test Steps
Testing this on Monday when I have a dev board handy.
Automated UTs are passing.
Checklist:
Related Issue
#1337
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.